-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IMP] data: align data versions to release versions #5973
Conversation
667257c
to
977f2a2
Compare
Task: 4659206
977f2a2
to
e0ff365
Compare
@@ -2,555 +2,188 @@ import { toZone } from "../../src/helpers/index"; | |||
|
|||
export const pivotModelData = function (xc: string) { | |||
return { | |||
version: 19, | |||
version: 25, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not strictly necessary, but I realized it after it was done so... 🤷♂️
migrate: (data: any) => any; | ||
} | ||
|
||
export function getCurrentVersion() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically, it's not the "current" version if there was no changes in one saas. Should it be rather named getLatestVersion
? or is it more confusing and getCurrentVersion
is clearer even if it's not strictly speaking 100% correct in all cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add an empty migration step if there is no changes in one saas version to keep consistency ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, I think we won't do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except if we add it to the freeze checklist :D
2b6df03
to
2d25706
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice !
Just a little suggestion, not sure about it but IMO it would be good to have the getCurrentVersion
matching the version of the branch
migrate: (data: any) => any; | ||
} | ||
|
||
export function getCurrentVersion() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add an empty migration step if there is no changes in one saas version to keep consistency ?
Typo in the first commit => |
This commit fixes two issues at once. Issue 1 ------- Currently, the migration process will ONLY run if the version of the original data (in json) is smaller than the last version set in the source code (`CURRENT_VERSION`). This means that any additional step set outside of the library is not run without incrementing CURRENT_VERSION in o-spreadsheet. Example: - we bump the latest version to 25 in o-spreadsheet; - we push that value in Odoo - users start creating spreadsheets in that version so their spreadsheets are set to version 25 - later on, we need to add a migration step for whatever reason in Odoo, we assign it to 25.1 => the migration code will check against the latest version in the source (25) vs the one in the spreadsheet data (25 as well) and will simply skip the migration. So 25.1 will never run... Issue 2 ------- The current scheme of the incremented integer is impractical when it comes to figure out which odoo version it matches(*). Knowing the release version is usefull for bug fixes to know which version of the code generated the json data. With this commit, data format version are now aligned with odoo versions Current master is 18.3. Next, it'll be 18.4, 19.0, 19.1, etc. (*) o-spreadsheet releases follow the same version numbers as odoo releases. Task: 4659206
2d25706
to
588f986
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
robodoo rebase-ff r+
Merge method set to rebase and fast-forward. |
Task: 4659206 Part-of: #5973 Signed-off-by: Pierre Rousseau (pro) <[email protected]>
This commit fixes two issues at once. Issue 1 ------- Currently, the migration process will ONLY run if the version of the original data (in json) is smaller than the last version set in the source code (`CURRENT_VERSION`). This means that any additional step set outside of the library is not run without incrementing CURRENT_VERSION in o-spreadsheet. Example: - we bump the latest version to 25 in o-spreadsheet; - we push that value in Odoo - users start creating spreadsheets in that version so their spreadsheets are set to version 25 - later on, we need to add a migration step for whatever reason in Odoo, we assign it to 25.1 => the migration code will check against the latest version in the source (25) vs the one in the spreadsheet data (25 as well) and will simply skip the migration. So 25.1 will never run... Issue 2 ------- The current scheme of the incremented integer is impractical when it comes to figure out which odoo version it matches(*). Knowing the release version is usefull for bug fixes to know which version of the code generated the json data. With this commit, data format version are now aligned with odoo versions Current master is 18.3. Next, it'll be 18.4, 19.0, 19.1, etc. (*) o-spreadsheet releases follow the same version numbers as odoo releases. closes #5973 Task: 4659206 Signed-off-by: Pierre Rousseau (pro) <[email protected]>
Description:
This commit fixes two issues at once.
Issue 1
Currently, the migration process will ONLY run if the version of the original
data (in json) is smaller than the last version set in the source code
(
CURRENT_VERSION
). This means that any additional step set outside of thelibrary is not run without incrementing CURRENT_VERSION in o-spreadsheet.
Example:
set to version 25
assign it to 25.1
=> the migration code will check against the latest version in the source (25)
vs the one in the spreadsheet data (25 as well) and will simply skip the
migration.
So 25.1 will never run...
Issue 2
The current scheme of the incremented integer is impractical when it comes
to figure out which odoo version it matches(*).
Knowing the release version is usefull for bug fixes to know which version of
the code generated the json data.
With this commit, data format version are now aligned with odoo versions
Current master is 18.3. Next, it'll be 18.4, 19.0, 19.1, etc.
(*) o-spreadsheet releases follow the same version numbers as odoo releases.
Task: 4659206
review checklist